Fix normalize_paths to allow whitespace#343
Fix normalize_paths to allow whitespace#343IanLee1521 merged 1 commit intoPyCQA:masterfrom willkg:normalize_path_fix
Conversation
This fixes normalize_paths so that it strips whitespace before and after paths before working on them. This allows you to do specify excludes with multiple lines in the config file. This also tweaks normalize_paths so that the behavior matches the docstring. This also adds tests. Fixes #339
There was a problem hiding this comment.
I split this up because if not value could be any falsey value and the docstring states that this function returns a list, thus it should return an empty list in these cases.
There was a problem hiding this comment.
Hi @willkg, I'd be interested to hear what your case was where this was failing for you. I'm tempted to update this to the following to handle the case where it isn't a list or a string coming in. Alternatively, perhaps it would be better to raise a ValueError at that point.
if not value:
return []
if isinstance(value, list):
return value
if not isinstance(value, str):
return []There was a problem hiding this comment.
I don't have a preference one way or the other. I changed this code because the docstring says it returns a list, but the code was just returning falsey values. I don't have a preference for my version or your version or even changing it to raise a ValueError.
|
Wouldn't most of your inline review comments be better as ... comments in the code? |
|
I didn't think so, which is why I did what I did. If someone wants me to move those reviewer comments into the code, I can. I wouldn't do that for my projects, but I'll do whatever to get this landed because long or non-trivial exclude configuration is very difficult to read and maintain right now. |
|
You're justifying your decisions for the code. Those belong as comments (unless you never document any code you write). That said, you might notice I don't have the fancy "contributor" label on my comments so appeasing me won't get your code merged, especially since the maintainer is not actively maintaining this right now. |
|
Generally, I agree with you regarding comments. Looking at these specific review comments, I don't think they belong in the code and don't think they help much. It's possible I don't understand what you're suggesting here since you've been pretty general so far. Can you go through the review comments I wrote up and translate them into code comments you think are appropriate here? |
|
@IanLee1521, are you the new maintainer? I haven't seen you on here before. |
|
Hi @myint, yes, I just started working today on finally getting some of these issues and pull requests resolved. |
4 participants
This fixes normalize_paths so that it strips whitespace before and after
paths before working on them. This allows you to do specify excludes
with multiple lines in the config file.
This also tweaks normalize_paths so that the behavior matches the
docstring.
This also adds tests.
Fixes #339